Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add Semantic Kernel Adapter documentation and usage examples in user guides #5256

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

ekzhu
Copy link
Collaborator

@ekzhu ekzhu commented Jan 29, 2025

Partially address #5205 and #5226

@ekzhu ekzhu requested review from lspinheiro and gagb January 29, 2025 21:47
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.40%. Comparing base (7020f2a) to head (d983b64).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5256   +/-   ##
=======================================
  Coverage   70.40%   70.40%           
=======================================
  Files         179      179           
  Lines       11617    11617           
=======================================
  Hits         8179     8179           
  Misses       3438     3438           
Flag Coverage Δ
unittests 70.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@lspinheiro lspinheiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There was a comment from @gagb about the docstring of the adapter being too long, it may be worth it keeping the examples in a user-guide/sample while maintaining a minimum example in the docstring. The code blocks are great for the reference API but I agree that when I try to hover over the docstring in vs code to understand some of the parameters it makes it quite harder to read. It is something that we could address either in this PR or in the future.

@ekzhu
Copy link
Collaborator Author

ekzhu commented Jan 30, 2025

LGTM. There was a comment from @gagb about the docstring of the adapter being too long, it may be worth it keeping the examples in a user-guide/sample while maintaining a minimum example in the docstring. The code blocks are great for the reference API but I agree that when I try to hover over the docstring in vs code to understand some of the parameters it makes it quite harder to read. It is something that we could address either in this PR or in the future.

I think API doc is quite useful. Different developers have different preferences, and you can look at Pandas, Numpy, Scikit-Learn, and PyTorch -- all of them use API docs as the main way to document their libraries.

I think there are 4 types of docs:

  1. API docs, explaining the usage of the function or the class being documentated, audience are developers that are already somewhat familiar with the library.
  2. Tutorial, explaining the usage for developers who have never used this library.
  3. Core concepts, explaining the high-level concepts necessary to understand the library and its usage scenarios
  4. Design docs, for us, the library developer

Right now, we have pretty good (2) and (3). But for (1) it is quite hit-and-miss -- we should improve this.

(4) is almost non-existent, but a bit lower priority for now.

I think samples are not documentation, but as "attachments" or "external links" from the documentation. They should focus on addressing a specific scenario that user may want to emulate. Using samples as the main documentation can potentially lead to developers not getting a complete picture of the API, and learn by trials-and-errors. However, some people learn like that, and we should still provide samples, just not designed to serve as the main entry point.

There is exception to the above point, that is when the library's API is extremely simple and straightforward. In that case, samples is great because you can cover most possible usages in a few samples. We should accept that our main libraries are no longer like that.

@ekzhu ekzhu merged commit 403844e into main Jan 30, 2025
66 checks passed
@ekzhu ekzhu deleted the ekzhu-sk-model-client-doc branch January 30, 2025 00:37
@lspinheiro
Copy link
Collaborator

LGTM. There was a comment from @gagb about the docstring of the adapter being too long, it may be worth it keeping the examples in a user-guide/sample while maintaining a minimum example in the docstring. The code blocks are great for the reference API but I agree that when I try to hover over the docstring in vs code to understand some of the parameters it makes it quite harder to read. It is something that we could address either in this PR or in the future.

I think API doc is quite useful. Different developers have different preferences, and you can look at Pandas, Numpy, Scikit-Learn, and PyTorch -- all of them use API docs as the main way to document their libraries.

I think there are 4 types of docs:

  1. API docs, explaining the usage of the function or the class being documentated, audience are developers that are already somewhat familiar with the library.
  2. Tutorial, explaining the usage for developers who have never used this library.
  3. Core concepts, explaining the high-level concepts necessary to understand the library and its usage scenarios
  4. Design docs, for us, the library developer

Right now, we have pretty good (2) and (3). But for (1) it is quite hit-and-miss -- we should improve this.

(4) is almost non-existent, but a bit lower priority for now.

I think samples are not documentation, but as "attachments" or "external links" from the documentation. They should focus on addressing a specific scenario that user may want to emulate. Using samples as the main documentation can potentially lead to developers not getting a complete picture of the API, and learn by trials-and-errors. However, some people learn like that, and we should still provide samples, just not designed to serve as the main entry point.

There is exception to the above point, that is when the library's API is extremely simple and straightforward. In that case, samples is great because you can cover most possible usages in a few samples. We should accept that our main libraries are no longer like that.

I don't disagree that the API docs are useful. In this case in particular case what I meant was that the extensive examples made it harder to look at parameter descriptions while coding. Checking the online docs to understand how to use the API and inspecting it into the IDE to know if I'm using a function correctly both fall into 1 and it is definitely a tradeoff there. Making it more detailed in the online docs make it harder to find specific information when we are hovering the docstrings in vs code.

I think it would be useful to have guidelines to make it more consistent. I think it is a hit-and-miss not only on which components are documented but also on documenting methods and params vs the classes.

@ekzhu
Copy link
Collaborator Author

ekzhu commented Jan 30, 2025

Agree. We need to have more consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants